http: fix failing test by ShogunPanda · Pull Request #43641 · nodejs/node

ShogunPanda · 2022-07-01T09:29:46Z

Fixes #43623.

github-actions · 2022-07-01T09:31:59Z

Fast-track has been requested by @ShogunPanda. Please 👍 to approve.

tniessen · 2022-07-01T09:43:55Z

@MoLow FYI, it looks like this is the same approach as #43633, except modifying two more occurrences in the file.

F3n67u · 2022-07-01T09:55:46Z

Fixes #43623.

@ShogunPanda Could you help me understand why this pr will fix the test failure?

tniessen · 2022-07-01T10:28:07Z

The test is still failing in CI.

If #43522 is the culprit, maybe revert (#43636) is the way to go? Looking at its CI history, it probably should not have landed: the test failed a bunch of times there, too.

tniessen · 2022-07-01T10:33:53Z

Unrelated side note: the commit message should start with http: fix ..., not http: fixed ....

ShogunPanda · 2022-07-01T10:44:37Z

We have lot of flakiness in tests lately. Please do not revert the other PR, I'll work in the next few hours to understand why it fails.

ShogunPanda · 2022-07-01T11:39:21Z

I found the issue. The problem was not in using the agent but in the fact that the server.close was called before the client could receive the response. As the request was completed closeIdleConnections would have closed it.
It should be fixed now.

nodejs-github-bot · 2022-07-01T11:39:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45027/

F3n67u · 2022-07-01T13:14:54Z

test-gc-http-client-timeout fail with Cannot read properties of null in Server.closeIdleConnections method, see #43638 (comment). This method is introduced by you, could you help to take a look? @ShogunPanda

F3n67u · 2022-07-01T13:23:14Z

test-gc-http-client-timeout fail with Cannot read properties of null in Server.closeIdleConnections method, see #43638 (comment). This method is introduced by you, could you help to take a look? @ShogunPanda

@ShogunPanda After some investigation, I found this is also caused by #43522. 😹
Can we reconsider reverting #43522?

ShogunPanda · 2022-07-01T13:25:58Z

My opinion is that after the changes we detect tests that were misbehaving on handling the socket.
I would suggest to rather fix those tests instead of simply revert the PR.

F3n67u · 2022-07-01T13:31:11Z

My opinion is that after the changes we detect tests that were misbehaving on handling the socket. I would suggest to rather fix those tests instead of simply revert the PR.

This pr causes two tests to fail, this will affect all pr. I would prefer to keep CI green.

When you fixed the test, you could land #43522 again. I don't see any downside to that. could you help me to understand why we would better not revert?

ShogunPanda · 2022-07-01T13:33:11Z

Because apparently CI was not detecting those tests to fail (not even locally) while somehow it is now. So I think we are in a better spot at the moment.

ShogunPanda · 2022-07-01T13:33:41Z

Moreover, can you share how you detect that the PR is involved? So I can investigate how to fix it

F3n67u · 2022-07-01T13:35:40Z

Because apparently CI was not detecting those tests to fail (not even locally) while somehow it is now. So I think we are in a better spot at the moment.

Ok. I will support you.

F3n67u · 2022-07-01T13:40:46Z

Moreover, can you share how you detect that the PR is involved? So I can investigate how to fix it

`test-stream-finished`

This test failure is root caused by @MoLow in #43623 (comment)

`test-gc-http-client-timeout`

The stack tell me error is happened at Server.closeIdleConnections in Server.close. I then find it is #43522 introduce Server.closeIdleConnections in Server.close.

done/collected/total: 93/0/804
done/collected/total: 322/93/804
done/collected/total: 415/322/804
done/collected/total: 664/349/804
done/collected/total: 737/415/804
done/collected/total: 804/664/804
done/collected/total: 804/737/804
done/collected/total: 804/804/804
node:_http_server:498
    connections[i].socket.destroy();
                          ^

TypeError: Cannot read properties of null (reading 'destroy')
    at Server.closeIdleConnections (node:_http_server:498:27)
    at Server.close (node:_http_server:481:8)
    at Immediate.status (/home/iojs/build/workspace/node-test-binary-armv7l/test/parallel/test-gc-http-client-timeout.js:66:14)
    at process.processImmediate (node:internal/timers:471:21)

ShogunPanda · 2022-07-01T13:42:47Z

Oh, you are on ARM. On MacOS I get a different error. But I suspect it is the same. Working on it.

ShogunPanda · 2022-07-01T14:18:48Z

I just have noticed that the test-gc-http-client-timeout has been created out of test-gc-http-client which was moved to sequential tests because it was not reliable. Shall we do the same?

github-actions · 2022-07-02T14:55:01Z

Fast-track has been requested by @tniessen. Please 👍 to approve.

F3n67u · 2022-07-02T14:55:10Z

Great. How can you be sure the flaky test is addressed? Do we need to run Jenkins ci more times to make sure the flaky test is fixed?

We have a special stress test Jenkins job for these situations, but it appears to be broken, presumably because the main branch was renamed recently: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/349/console

wow. this tool is great. I don't know that before.

mcollina

lgtm

nodejs-github-bot · 2022-07-02T16:01:16Z

Commit Queue failed

- Loading data for nodejs/node/pull/43641 ✔ Done loading data for nodejs/node/pull/43641 ----------------------------------- PR info ------------------------------------ Title http: fix failing test (#43641) Author Paolo Insogna (@ShogunPanda) Branch ShogunPanda:fix-failing-test -> nodejs:main Labels test, fast-track, needs-ci Commits 2 - http: fix failing test - http: moved test to sequential Committers 1 - Paolo Insogna PR-URL: https://github.com/nodejs/node/pull/43641 Reviewed-By: Matteo Collina Reviewed-By: Filip Skokan ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/43641 Reviewed-By: Matteo Collina Reviewed-By: Filip Skokan -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 01 Jul 2022 09:29:46 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43641#pullrequestreview-1026765760 ✔ - Filip Skokan (@panva): https://github.com/nodejs/node/pull/43641#pullrequestreview-1026768532 ℹ This PR is being fast-tracked ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-07-02T04:04:05Z: https://ci.nodejs.org/job/node-test-pull-request/45045/ - Querying data for job/node-test-pull-request/45045/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 43641 From https://github.com/nodejs/node * branch refs/pull/43641/merge -> FETCH_HEAD ✔ Fetched commits as 201460873d07..64cf037bbed1 -------------------------------------------------------------------------------- [main f57c58aa05] http: fix failing test Author: Paolo Insogna Date: Fri Jul 1 13:39:05 2022 +0200 1 file changed, 3 insertions(+), 1 deletion(-) [main 344ac7b8a0] http: moved test to sequential Author: Paolo Insogna Date: Fri Jul 1 16:29:04 2022 +0200 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{parallel => sequential}/test-gc-http-client-timeout.js (100%) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: fix failing test

PR-URL: #43641
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

[detached HEAD 0cc5cfe164] http: fix failing test
Author: Paolo Insogna paolo@cowtech.it
Date: Fri Jul 1 13:39:05 2022 +0200
1 file changed, 3 insertions(+), 1 deletion(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: moved test to sequential

PR-URL: #43641
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

[detached HEAD 91de97a73b] http: moved test to sequential
Author: Paolo Insogna paolo@cowtech.it
Date: Fri Jul 1 16:29:04 2022 +0200
1 file changed, 0 insertions(+), 0 deletions(-)
rename test/{parallel => sequential}/test-gc-http-client-timeout.js (100%)

Successfully rebased and updated refs/heads/main.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/2602070703

nodejs-github-bot · 2022-07-02T16:10:55Z

Landed in c753f27

F3n67u · 2022-07-03T01:12:59Z

Great. How can you be sure the flaky test is addressed? Do we need to run Jenkins ci more times to make sure the flaky test is fixed?

We have a special stress test Jenkins job for these situations, but it appears to be broken, presumably because the main branch was renamed recently: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/349/console

@tniessen Could we file an issue to nodejs/build or nodejs/node to record this bug?

I tried to fix it to the main branch in nodejs/build repo, but I cannot figure out where the Jenkins file of node-stress-single-test is(I think it resides in some private repo of building wg). Nonetheless, I create nodejs/build#2986 and nodejs/build#2987 to update a lot of master branch usage to main.

F3n67u · 2022-07-03T05:54:56Z

@ShogunPanda test-gc-http-client-timeout is still flaky after move to sequential: https://ci.nodejs.org/job/node-test-commit-arm/42478/nodes=ubuntu2004-armv7l/testReport/junit/(root)/test/sequential_test_gc_http_client_timeout/

ShogunPanda · 2022-07-03T06:01:05Z

@F3n67u I'll soon file a request to @nodejs/build to get access to machine and investigate it. On macOS it does not happen

F3n67u · 2022-07-04T15:01:30Z

@F3n67u I'll soon file a request to @nodejs/build to get access to machine and investigate it. On macOS it does not happen

Great. thank you.

PR-URL: #43641 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 1, 2022

ShogunPanda added the fast-track PRs that do not need to wait for 48 hours to land. label Jul 1, 2022

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2022

This comment was marked as outdated.

Sign in to view

tniessen mentioned this pull request Jul 1, 2022

Investigate flaky test-stream-finished #43623

Closed

http: fix failing test

d62be4d

ShogunPanda force-pushed the fix-failing-test branch from 766c2e5 to d62be4d Compare July 1, 2022 11:39

F3n67u removed the fast-track PRs that do not need to wait for 48 hours to land. label Jul 1, 2022

ShogunPanda mentioned this pull request Jul 1, 2022

Revert "http: use Keep-Alive by default in global agents" #43636

Closed

http: moved test to sequential

64cf037

tniessen requested a review from mcollina July 2, 2022 14:41

tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Jul 2, 2022

mcollina approved these changes Jul 2, 2022

View reviewed changes

panva approved these changes Jul 2, 2022

View reviewed changes

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 2, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 2, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 2, 2022

nodejs-github-bot merged commit c753f27 into nodejs:main Jul 2, 2022

ShogunPanda deleted the fix-failing-test branch July 2, 2022 16:45

F3n67u mentioned this pull request Jul 3, 2022

src: fix crash on FSReqPromise destructor #43533

Merged

MoLow mentioned this pull request Jul 3, 2022

test: fix test stream finished #43633

Closed

github-actions bot mentioned this pull request Jul 4, 2022

CI Reliability 2022-07-04 nodejs/reliability#317

Open

23 tasks

ShogunPanda mentioned this pull request Jul 8, 2022

Access to @ShogunPanda to ubuntu2004-armv7l nodejs/build#2992

Closed

lpinca mentioned this pull request Jul 10, 2022

test: mark test-gc-http-client-timeout as flaky on arm #43754

Closed

targos pushed a commit that referenced this pull request Jul 12, 2022

http: fix failing test

e6d4837

PR-URL: #43641 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

F3n67u mentioned this pull request Jul 12, 2022

Server.closeIdleConnections break gc tests #43795

Closed

targos added the dont-land-on-v16.x label Jul 31, 2022

Uh oh!

Conversation

ShogunPanda commented Jul 1, 2022

Uh oh!

github-actions bot commented Jul 1, 2022

Uh oh!

tniessen commented Jul 1, 2022

Uh oh!

This comment was marked as outdated.

F3n67u commented Jul 1, 2022

Uh oh!

tniessen commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jul 1, 2022

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

nodejs-github-bot commented Jul 1, 2022

Uh oh!

F3n67u commented Jul 1, 2022

Uh oh!

F3n67u commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

F3n67u commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

F3n67u commented Jul 1, 2022

Uh oh!

F3n67u commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

test-stream-finished

test-gc-http-client-timeout

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

ShogunPanda commented Jul 1, 2022

Uh oh!

github-actions bot commented Jul 2, 2022

Uh oh!

F3n67u commented Jul 2, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 2, 2022

PR-URL: #43641 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Filip Skokan panva.ip@gmail.com

PR-URL: #43641 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Filip Skokan panva.ip@gmail.com

Successfully rebased and updated refs/heads/main.

Uh oh!

nodejs-github-bot commented Jul 2, 2022

Uh oh!

F3n67u commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

F3n67u commented Jul 3, 2022

Uh oh!

ShogunPanda commented Jul 3, 2022

Uh oh!

F3n67u commented Jul 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tniessen commented Jul 1, 2022 •

edited

Loading

F3n67u commented Jul 1, 2022 •

edited

Loading

F3n67u commented Jul 1, 2022 •

edited

Loading

F3n67u commented Jul 1, 2022 •

edited

Loading

`test-stream-finished`

`test-gc-http-client-timeout`

PR-URL: #43641
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

PR-URL: #43641
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Filip Skokan panva.ip@gmail.com

F3n67u commented Jul 3, 2022 •

edited

Loading